-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(mappers): Stream name can now be accessed in __alias__
context of stream maps
#2701
base: main
Are you sure you want to change the base?
feat(mappers): Stream name can now be accessed in __alias__
context of stream maps
#2701
Conversation
CodSpeed Performance ReportMerging #2701 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2701 +/- ##
=======================================
Coverage 90.50% 90.50%
=======================================
Files 62 62
Lines 4994 4994
Branches 974 974
=======================================
Hits 4520 4520
Misses 328 328
Partials 146 146 ☔ View full report in Codecov by Sentry. |
singer_sdk/mapper.py
Outdated
except simpleeval.NameNotDefined: | ||
logging.debug( | ||
"Failed to evaluate simpleeval expression %(expr) - " | ||
"falling back to original expression", | ||
extra={"expr": expr}, | ||
) | ||
result: str = expr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am swallowing NameNotDefined errors here to support existing __alias__
configurations. Since __alias__
was not previously evaluated, it contains plain strings - which the evaluator tries to read as python objects.
For instance, { "__alias__": "aliased_stream" }
will produce NameNotDefined
. It would need single quotes to avoid this, like { "__alias__": "'aliased_stream'" }
, which is a breaking change. Instead I'm just passing the expression as a string in these cases.
Please let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense. Thanks for being mindful of breaking changes 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks @holly-evans!
Co-authored-by: Edgar Ramírez Mondragón <16805946+edgarrmondragon@users.noreply.github.com>
__alias__
context of stream maps
Description
Allow users to reference the stream name in stream maps alias fields, under a special key:
Related
__alias__
was not an evaluated field before now📚 Documentation preview 📚: https://meltano-sdk--2701.org.readthedocs.build/en/2701/